Skip to content

feat: lock on email, not session ID#100

Merged
joaquimds merged 2 commits into
masterfrom
feat/lock-on-email
May 14, 2026
Merged

feat: lock on email, not session ID#100
joaquimds merged 2 commits into
masterfrom
feat/lock-on-email

Conversation

@joaquimds
Copy link
Copy Markdown
Member

No description provided.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the join/webhook concurrency strategy to lock per person (email) rather than per request/session, to reduce race conditions across /join and concurrent webhook deliveries for the same member.

Changes:

  • Replaced session-token locking with a generalized JoinService::acquireLock($key) / releaseLock() keyed primarily by email (with sessionToken fallback).
  • Added per-email locking to Stripe webhook handling to serialize CRM mutations for a single customer.
  • Updated the session lock concurrency test to use an email-shaped lock key and safer shell escaping.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/join-block/src/Services/JoinService.php Introduces generalized per-key locking and updates /join to lock by email (fallback to sessionToken).
packages/join-block/src/Services/StripeService.php Acquires/releases the per-email lock around webhook flows that mutate CRM state.
packages/join-block/src/Services/ActionNetworkService.php Adds tag-existence checks by fetching a person’s tags prior to add/remove operations.
packages/join-block/tests/SessionLockTest.php Updates the locking test to validate serialization by an email-shaped key and escapes shell args.
packages/join-block/tests/SessionLockTestProcess.php Updates the child process to use acquireLock()/releaseLock() and key-based logging.
Comments suppressed due to low confidence (2)

packages/join-block/src/Services/ActionNetworkService.php:186

  • Inside getPersonTagNames(), the code issues an additional HTTP request per tagging to resolve the tag resource (GET $tagHref). This is an N+1 request pattern and can significantly slow down webhook/join requests and increase the chance of hitting Action Network rate limits. Consider using data already present in the taggings response (if available), batching, caching tag href->name lookups, or restructuring to avoid fetching every tag when callers only need to test membership for a specific tag.
            foreach ($taggings as $tagging) {
                $tagHref = $tagging["_links"]["osdi:tag"]["href"] ?? null;
                if (!$tagHref) {
                    continue;
                }
                $tagResponse = $client->request("GET", $tagHref, ["headers" => $headers]);
                $tagBody = json_decode($tagResponse->getBody()->getContents(), true);
                if (!empty($tagBody["name"])) {
                    $tagNames[] = $tagBody["name"];
                }

packages/join-block/src/Services/ActionNetworkService.php:208

  • addTag()/removeTag() now call getPersonTagNames() which fetches all tags for the person before adding/removing a single tag. This makes each tag mutation O(total tags) network calls and work. A more scalable approach is a personHasTag($email, $tag) helper that short-circuits as soon as the target tag is found (and avoids resolving unrelated tags).
        $tagNames = self::getPersonTagNames($email);
        if ($tagNames === null) {
            $joinBlockLog->warning("Skipping Action Network addTag('$tag') for $email: person does not exist");
            return;
        }
        if (in_array($tag, $tagNames, true)) {
            $joinBlockLog->info("Skipping Action Network addTag('$tag') for $email: tag already applied");
            return;
        }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +173 to +176
while ($nextHref) {
$response = $client->request("GET", $nextHref, ["headers" => $headers]);
$body = json_decode($response->getBody()->getContents(), true);
$taggings = $body["_embedded"]["osdi:taggings"] ?? [];
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (3)

packages/join-block/src/Services/ActionNetworkService.php:178

  • In getPersonTagNames(), the initial self::getPerson($email) call happens outside the try/catch. If Action Network returns a non-2xx or there’s a network error, Guzzle will throw and we’ll never hit the intended fallback (TAG_ENUMERATION_ABORTED) — addTag/removeTag will fail instead of proceeding with the no-op POST. Consider wrapping getPerson() in the same try/catch (or catching RequestException) and returning TAG_ENUMERATION_ABORTED on failure.
        $person = self::getPerson($email);
        if ($person === null) {
            return null;
        }

packages/join-block/src/Services/ActionNetworkService.php:219

  • getPersonTagNames() can issue a very large number of HTTP requests: up to MAX_TAGGING_PAGES taggings-page requests, plus an additional GET per tagging to fetch the tag resource just to read its name. This is potentially more expensive than the prior behavior (single POST no-op) and will also extend the time we hold the per-email JoinService lock. Consider avoiding per-tag fetches (e.g., use tag data already embedded in taggings if available, request an expanded/embedded tag representation, or skip enumeration and rely on Action Network’s no-op semantics).
                $response = $client->request("GET", $nextHref, $requestOptions);
                $body = json_decode($response->getBody()->getContents(), true);
                $taggings = $body["_embedded"]["osdi:taggings"] ?? [];
                foreach ($taggings as $tagging) {
                    $tagHref = $tagging["_links"]["osdi:tag"]["href"] ?? null;
                    if (!$tagHref) {
                        continue;
                    }
                    $tagResponse = $client->request("GET", $tagHref, $requestOptions);
                    $tagBody = json_decode($tagResponse->getBody()->getContents(), true);
                    if (!empty($tagBody["name"])) {
                        $tagNames[] = $tagBody["name"];
                    }
                }

packages/join-block/src/Services/ActionNetworkService.php:158

  • getPerson() doesn’t set any timeout/connect_timeout options, even though HTTP_TIMEOUT_SECONDS is introduced and used elsewhere. A slow/hung Action Network request here can block the join/webhook flow (and any per-email lock). Consider applying the same timeout/connect_timeout options used in getPersonTagNames() to this request as well.
        $response = $client->request(
            "GET",
            "https://actionnetwork.org/api/v2/people/",
            [
                "headers" => [
                    "OSDI-API-Token" => Settings::get("ACTION_NETWORK_API_KEY")
                ],
                "query" => $query
            ]
        );

Comment on lines 142 to 144
$query = [
"filter" => "email_address eq '" . $email . "'"
];
Comment on lines 38 to 50
// phpcs:ignore WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents
$logs = file_get_contents($logFile);
# The two processes use the same session lock, so they must not overlap.
# The two processes use the same lock key, so they must not overlap.
# If they ran in parallel the log would read WORKING -> WORKING -> DONE -> DONE.
# Sequential execution instead reads WORKING -> DONE -> WORKING -> DONE
# (the second process can only start WORKING once the first has released the lock).
# The session id is included on each line to scope the match to this test run.
# The lock key is included on each line to scope the match to this test run.
# We don't assert on the "Unlocked" line here: it is logged after flock() releases
# the lock, so the next process can legitimately log "WORKING" before it appears.
$quotedKey = preg_quote($lockKey, '#');
$matched = preg_match(
"#WORKING $sessionId.*DONE $sessionId.*WORKING $sessionId.*DONE $sessionId#s",
"#WORKING $quotedKey.*DONE $quotedKey.*WORKING $quotedKey.*DONE $quotedKey#s",
$logs
Comment on lines 837 to +840

$joinBlockLog->info("Syncing updated customer details for Stripe customer {$customer['id']} ($email)");

$lockFile = JoinService::acquireLock($email);
@joaquimds joaquimds merged commit 91b191a into master May 14, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants